refactor: move package-header to new component#2030
refactor: move package-header to new component#2030alexdln wants to merge 2 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis pull request extracts the package page header into a new Vue 3 component at app/components/Package/Header.vue, and introduces a composable usePackageHeaderHeight to expose the header height as shared state. The package page (app/pages/package/[[org]]/[name].vue) is updated to use the new component, removing inline header DOM, clipboard copy logic, scroll-to-top handling, likes logic, and related header/footer observers. An accessibility test for PackageHeader is added to the a11y suite. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b2f1f42-533d-4182-aa80-4a05674df696
📒 Files selected for processing (3)
app/components/Package/Header.vueapp/composables/usePackageHeaderHeight.tsapp/pages/package/[[org]]/[name].vue
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/nuxt/a11y.spec.ts (1)
729-757: The new a11y case skips the moved action states.With
docsLink/codeLinkset tonullandlatestVersionmatchingdisplayVersion, this never renders the header actions that were moved out of the page. A regression in those links/buttons would still pass the suite.Suggested follow-up
describe('PackageHeader', () => { + const baseProps = { + pkg: { name: 'vue' }, + resolvedVersion: '3.5.0', + displayVersion: { + _id: '1234567890', + _npmVersion: '3.5.0', + name: 'vue', + version: '3.5.0', + dist: { + shasum: '1234567890', + signatures: [], + tarball: 'https://npmx.dev/package/vue/tarball', + }, + }, + latestVersion: { version: '3.5.0', tags: [] }, + provenanceData: null, + provenanceStatus: 'idle', + docsLink: null, + codeLink: null, + isBinaryOnly: false, + } + it('should have no accessibility violations', async () => { const component = await mountSuspended(PackageHeader, { - props: { - pkg: { name: 'vue' }, - resolvedVersion: '3.5.0', - displayVersion: { - _id: '1234567890', - _npmVersion: '3.5.0', - name: 'vue', - version: '3.5.0', - dist: { - shasum: '1234567890', - signatures: [], - tarball: 'https://npmx.dev/package/vue/tarball', - }, - }, - latestVersion: { version: '3.5.0', tags: [] }, - provenanceData: null, - provenanceStatus: 'idle', - docsLink: null, - codeLink: null, - isBinaryOnly: false, - }, + props: baseProps, }) const results = await runAxe(component) expect(results.violations).toEqual([]) }) + + it('should have no accessibility violations with header actions visible', async () => { + const component = await mountSuspended(PackageHeader, { + props: { + ...baseProps, + latestVersion: { version: '3.6.0', tags: ['latest'] }, + docsLink: { name: 'docs', params: { path: ['vue', 'v', '3.5.0'] } }, + codeLink: { + name: 'code', + params: { packageName: 'vue', version: '3.5.0', filePath: '' }, + }, + }, + }) + const results = await runAxe(component) + expect(results.violations).toEqual([]) + }) })As per coding guidelines,
**/*.{test,spec}.{ts,tsx}: Write unit tests for core functionality usingvitest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6512c139-fb07-4673-97ec-6fffbd7b5d60
📒 Files selected for processing (2)
app/pages/package/[[org]]/[name].vuetest/nuxt/a11y.spec.ts
🧭 Context
I'm working on the header updating to the new UI. To avoid making too big PR, I'm currently creating a separate PR to move the header into a separate component
I'll create a prs train next, but we can merge this part right away to make things easier for others and to reduce conflicts
Essentially, it's almost just a move. The only interesting part is package-header height calculations via global state, making it easier to share between components (I'm thinking of converting to CSS-variable later, but for now, this is better and closer to original approach)